Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: No error message when uploading a file that is not allowed #31346

Merged
merged 3 commits into from
Jan 2, 2024

Conversation

Sayan4444
Copy link
Contributor

@Sayan4444 Sayan4444 commented Dec 30, 2023

Proposed changes (including videos or screenshots)

BEFORE THE FIX
https://github.com/RocketChat/Rocket.Chat/assets/112304873/68e9d8d0-3ef5-411a-93e5-d009ff97c331

AFTER THE FIX
https://github.com/RocketChat/Rocket.Chat/assets/112304873/73c161a2-3658-4b76-8276-e38dbfc6f177

REASON BEHIND CHANGES
If the type of file which we want to upload is not clear then we should not allow it to be uploaded in the server

Issue(s)
This should close issues :-
closes #27924

Steps to test or reproduce

  1. Login to rocket chat.
  2. Set "Accepted Media Types" setting in "File Upload" section. For example image/*,application/pdf,text/plain,application/msword
  3. Try sending a rare file type like .har
  4. The file is not loaded into the chat, and there is no error message.

@Sayan4444 Sayan4444 requested a review from a team as a code owner December 30, 2023 11:16
Copy link

changeset-bot bot commented Dec 30, 2023

🦋 Changeset detected

Latest commit: d92f22d

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 30 packages
Name Type
@rocket.chat/meteor Patch
@rocket.chat/core-typings Patch
@rocket.chat/rest-typings Patch
@rocket.chat/core-services Patch
@rocket.chat/cron Patch
@rocket.chat/gazzodown Patch
@rocket.chat/livechat Patch
@rocket.chat/model-typings Patch
@rocket.chat/ui-contexts Patch
@rocket.chat/account-service Patch
@rocket.chat/authorization-service Patch
@rocket.chat/ddp-streamer Patch
@rocket.chat/omnichannel-transcript Patch
@rocket.chat/presence-service Patch
@rocket.chat/queue-worker Patch
@rocket.chat/stream-hub-service Patch
@rocket.chat/api-client Patch
@rocket.chat/license Patch
@rocket.chat/omnichannel-services Patch
@rocket.chat/pdf-worker Patch
@rocket.chat/presence Patch
rocketchat-services Patch
@rocket.chat/ddp-client Patch
@rocket.chat/fuselage-ui-kit Patch
@rocket.chat/models Patch
@rocket.chat/ui-client Patch
@rocket.chat/ui-video-conf Patch
@rocket.chat/uikit-playground Patch
@rocket.chat/web-ui-registration Patch
@rocket.chat/instance-status Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link

codecov bot commented Dec 30, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (92a8de7) 49.36% compared to head (d92f22d) 49.36%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff            @@
##           develop   #31346   +/-   ##
========================================
  Coverage    49.36%   49.36%           
========================================
  Files         3311     3308    -3     
  Lines        81334    81294   -40     
  Branches     16657    16648    -9     
========================================
- Hits         40148    40131   -17     
+ Misses       36487    36472   -15     
+ Partials      4699     4691    -8     
Flag Coverage Δ
e2e 52.57% <100.00%> (-0.02%) ⬇️
e2e-api 40.67% <ø> (+0.01%) ⬆️
unit 76.64% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

hugocostadev
hugocostadev previously approved these changes Jan 2, 2024
Copy link
Contributor

@hugocostadev hugocostadev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🚀

@dionisio-bot dionisio-bot bot added stat: ready to merge PR tested and approved waiting for merge and removed stat: needs QA labels Jan 2, 2024
@dionisio-bot dionisio-bot bot added stat: ready to merge PR tested and approved waiting for merge and removed stat: ready to merge PR tested and approved waiting for merge labels Jan 2, 2024
@kodiakhq kodiakhq bot merged commit 9ef1442 into RocketChat:develop Jan 2, 2024
43 checks passed
@Sayan4444 Sayan4444 deleted the uploadFile branch January 2, 2024 14:42
gabriellsh added a commit that referenced this pull request Jan 3, 2024
…hideUi

* 'develop' of github.com:RocketChat/Rocket.Chat: (112 commits)
  chore: update yarn.lock
  chore: Replace `FramedIcon` in favor of fuselage component (#31356)
  chore: fix dep version
  Release 6.5.2
  fix: wrong value used for workspace registration
  ci: update release PR description with change log
  fix: inquiries being limited to 50 items (#31267)
  fix: close image gallery on backdrop click (#31344)
  fix: No error message when uploading a file that is not allowed (#31346)
  fix: Text area only shows the last line of the message for very long texts #31252 #31257 (#31262)
  chore: adding new streamer event for imported messages (#31063)
  chore: adapt read receipts feature to work without oplog on msgs (#30996)
  chore: remove Messages model dependency of Rooms model (#31272)
  chore: `Card` normalization (#31335)
  refactor: unify useStream and useSingleStream (#31326)
  chore: add new msg stream for reported msgs (#31087)
  Bump 6.5.2
  feat: Highlight words in thread messages (#31281)
  fix: Show only departments that are allowed on registration page on livechat widget (#31302)
  fix: Fallback forward department not working as expected on chat forwarding (#31296)
  ...
@IgorOhrimenko
Copy link

IgorOhrimenko commented Feb 21, 2024

This merge broke the uploading of any files with an unknown type:
#31692
Set the default value in the settings:
изображение
and create an empty file with no extension or an .ovpn extension, try to upload and you will receive an error.

@emoxam
Copy link

emoxam commented Feb 21, 2024

"If the type of file which we want to upload is not clear then we should not allow it to be uploaded in the server" - We don't need it!

@IgorOhrimenko
Copy link

Default value is Leave it blank for accepting all media types.
I leave it blank and I want to uploading qwe.ovpn file and I cannot. Why?

@Sayan4444
Copy link
Contributor Author

@IgorOhrimenko the problem is that previously when u try to upload something whose file type cant be determined, not only the file was not uploaded but no error was given....thus this commit throws the error to let the end user know that this file cant be uploaded in the backend

@emoxam
Copy link

emoxam commented Feb 21, 2024

Ok. How to upload OVPN file ?

@roman-bergman
Copy link

I check mimetype for *.ovpn file. It has next type: text/plain
ovpn mimetype

I added text/plain to Accepted Media Types and but i get the same error: "media types not accepted".
I tried reboot my docker container and vm. It didn't produce any results.

@emoxam
Copy link

emoxam commented Feb 29, 2024

ovpn file is text/plain
the same has txt files..

Can we turn off media type checking at all ?

@DustpaN-Spb
Copy link

DustpaN-Spb commented Mar 4, 2024

Look like a last change is a error
Because
Was: invalidContentType: Boolean(file.type && !fileUploadIsValidContentType(file.type)),
Now: invalidContentType: !(file.type && fileUploadIsValidContentType(file.type)),
If we can't detrminate a MIME type of a file (file.type) it will return a false and all not recognised types will be not work at all even if "Accepted Media Types" is empty.
Please revert
https://forums.rocket.chat/t/cant-upload-ovpn-file/19455

@emoxam
Copy link

emoxam commented Mar 7, 2024

Look like a last change is a error Because Was: invalidContentType: Boolean(file.type && !fileUploadIsValidContentType(file.type)), Now: invalidContentType: !(file.type && fileUploadIsValidContentType(file.type)), If we can't detrminate a MIME type of a file (file.type) it will return a false and all not recognised types will be not work at all even if "Accepted Media Types" is empty. Please revert https://forums.rocket.chat/t/cant-upload-ovpn-file/19455

Where it was ? What path and file you are talking about ? Who should revert it ?

@DustpaN-Spb
Copy link

In this merge in tab "Files changed" you can see a changed files.
Author of a commit should do it?
I do not know how all work here.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we can't detrminate a MIME type of a file (file.type) it will return a false and all not recognised types will be not work at all even if "Accepted Media Types" is empty.
Please revert

@richardturnerfos
Copy link

How do I manage the mime types to allow my files?

@D34DC3N73R
Copy link

I'd rather have no error message when uploading file types that are not allowed, than not be able to upload unrecognized MIME types when no restrictions are set. This should be reverted.

@emoxam
Copy link

emoxam commented May 22, 2024

I'd rather have no error message when uploading file types that are not allowed, than not be able to upload unrecognized MIME types when no restrictions are set. This should be reverted.

Totally agree!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
communityPR stat: QA skipped stat: ready to merge PR tested and approved waiting for merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No error message when uploading a file that is not allowed
8 participants